Add HTTPS to Sidecar for deployment #8
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe latest updates enhance both local development and deployment processes, incorporating new instructions and configurations for a smoother setup and execution. Security is bolstered through updated private keys and hash values in configuration files. Additionally, improvements in error handling within the sidecar application ensure more robust operations by introducing a new exception class and expanding subprocess execution capabilities with a customizable working directory. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to path filters (8)
local_dev/config/network.tomlis excluded by:!**/*.tomlserver/supabase/config.tomlis excluded by:!**/*.tomlsidecar/traefik/dynamic-local/dynamic.yamlis excluded by:!**/*.yamlsidecar/traefik/dynamic-local/tls.yamlis excluded by:!**/*.yamlsidecar/traefik/dynamic/dynamic.yamlis excluded by:!**/*.yamlsidecar/traefik/dynamic/tls.yamlis excluded by:!**/*.yamlsidecar/traefik/traefik-local.yamlis excluded by:!**/*.yamlsidecar/traefik/traefik.yamlis excluded by:!**/*.yaml
Files selected for processing (14)
- .gitignore (1 hunks)
- README.md (3 hunks)
- server/app/auth/callback/route.ts (1 hunks)
- server/app/login/GitHubOAuthComponent.tsx (1 hunks)
- server/app/query/servers.tsx (2 hunks)
- sidecar/app/helpers.py (2 hunks)
- sidecar/app/main.py (1 hunks)
- sidecar/app/query/command.py (4 hunks)
- sidecar/app/query/ipa.py (7 hunks)
- sidecar/app/query/step.py (1 hunks)
- sidecar/app/routes/start.py (1 hunks)
- sidecar/app/routes/stop.py (2 hunks)
- sidecar/app/settings.py (3 hunks)
- sidecar/cli/cli.py (5 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Additional comments: 21
sidecar/app/main.py (1)
- 11-11: The update to the CORS allowed origins to include two new URLs (
https://draft.testandhttps://draft-mpc.vercel.app) is a positive change for enabling more flexible testing environments. However, ensure that these URLs are appropriate for the environments where the application will be deployed and that they do not inadvertently expose the application to unwanted origins.sidecar/app/settings.py (2)
- 20-20: Adding
helper_portas an integer attribute to theSettingsclass is a clear and straightforward way to configure the port for helper services. This change should make it easier to manage and adjust the port settings as needed.- 20-20: The refactoring of the
_helpersattribute initialization to use theload_helpers_from_network_configfunction directly in themodel_post_initmethod simplifies the process of loading helper configurations. This approach enhances maintainability by centralizing the logic for helper initialization.sidecar/app/routes/stop.py (1)
- 8-8: Removing validation of query signatures from the
finishendpoint simplifies the endpoint's logic but raises concerns about the security implications of this change. Ensure that alternative security measures are in place to authenticate and authorize requests to this endpoint, especially since it involves stopping queries.server/app/login/GitHubOAuthComponent.tsx (1)
- 15-17: Updating the default URL to
https://draft.test/and ensuring it starts withhttps://are important steps for enhancing the security of the OAuth flow. This change ensures that the redirect URL used in the GitHub login process is secure. However, consider validating theurlvariable to ensure it is a valid URL format to prevent potential security vulnerabilities.server/app/auth/callback/route.ts (1)
- 11-16: The modifications to the
originvariable declaration to include default values based on environment variables and ensure it starts with "https://" are prudent for enhancing the security and flexibility of the authentication callback route. This change ensures that redirects are performed to secure URLs. Similar to the previous file, consider validating theoriginvariable to ensure it is a valid URL format to prevent potential security vulnerabilities.sidecar/app/helpers.py (2)
- 21-22: The update to use
ParseResultobjects forhelper_urlandsidecar_urlin theHelperclass is a significant improvement in simplifying the URL handling logic. This change makes the code more readable and maintainable by leveraging the standard library's URL parsing capabilities. Ensure that all usages of these URLs properly handle theParseResultformat.- 33-34: The refactoring to parse URLs using
urlparseand directly assign them asParseResultobjects tohelper_urlandsidecar_urlattributes in theload_helpers_from_network_configfunction is a clean and efficient approach. This ensures consistency in URL handling across the application. Double-check that all parts of the application that interact with these URLs are updated to work with theParseResultformat.Also applies to: 41-42, 46-47, 55-56
sidecar/app/query/step.py (1)
- 83-83: The comment regarding the potential deletion of the
envattribute from theCommandStepclass raises a question about the necessity of this attribute. If theenvattribute is not being used or if there's a more efficient way to manage environment variables for commands, consider following through with this suggestion. However, ensure that any changes do not impact the functionality of command execution, especially in terms of environment variable dependencies.sidecar/app/routes/start.py (1)
- 56-56: Updating the reference to
settings.helper_portin thestart_ipa_helperfunction is a straightforward change that aligns with the modifications in theSettingsclass. This ensures that the port configuration for the IPA helper query is correctly sourced from the updated settings. Ensure that all references to the helper port throughout the application are consistently updated to reflect this change.sidecar/app/query/command.py (1)
- 19-19: Adding a
cwdfield of typePathto theCommandclass with a default value ofNoneis a valuable enhancement that provides flexibility in specifying the current working directory for subprocess execution. This change allows for more precise control over the execution environment of commands, which can be particularly useful in complex deployment scenarios. Ensure that thecwdfield is properly utilized in all relevant command executions.sidecar/cli/cli.py (4)
- 20-21: Refactoring the
start_helper_sidecar_commandfunction to remove the dependency onload_helpers_from_network_configand adjusting the logic related to roles and paths improves the clarity and maintainability of the CLI. The addition ofhelper_portandsidecar_portas parameters enhances the flexibility of the sidecar setup. Ensure that the CLI documentation and help messages are updated to reflect these new options.Also applies to: 40-41
- 47-61: Introducing the
start_traefik_commandfunction to handle Traefik setup is a significant addition that aligns with the PR's objective of integratingtraefikfor TLS handling. The function's design, which includes parameters for configuring ports and domains, provides flexibility for deployment configurations. Ensure that the Traefik configuration file (traefik.yaml) is properly set up to work with these dynamic parameters.- 64-82: The
start_traefik_local_commandfunction, designed for local development setups, is a thoughtful addition that complements thestart_traefik_commandfunction. The use of a tuple forsidecar_portsand the inclusion of aserver_portparameter allow for a highly configurable local development environment. As with the previous function, ensure that the Traefik configuration file (traefik-local.yaml) is correctly set up to utilize these parameters.- 91-124: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [85-121]
The refactoring and addition of new CLI commands and options, including those for Traefik setup and parallel command execution, significantly enhance the CLI's functionality and the application's deployment flexibility. Ensure that comprehensive testing is conducted to verify the correct behavior of these new commands and options in various deployment scenarios. Additionally, consider updating the CLI documentation to provide clear usage instructions and examples for these new features.
Also applies to: 132-169
README.md (2)
- 102-134: The instructions for installing Traefik, updating the hosts file, and generating local certificates are clear and concise. However, it's important to emphasize the security implications of using self-signed certificates for local development. Users should be aware that self-signed certificates can trigger warnings in browsers and are not suitable for production environments.
- Consider adding a note about the security implications of using self-signed certificates for local development and the importance of using certificates from a trusted CA for production environments.
- 196-212: The instructions for making configuration changes are clear, but there's a missed opportunity to emphasize the importance of secure configuration practices. Specifically, when moving Let's Encrypt keys and certificates into place, it's crucial to ensure that file permissions are set correctly to prevent unauthorized access.
- Suggest adding guidance on setting appropriate file permissions for TLS certificates and keys to ensure they are not readable by unauthorized users. This is a critical step in securing the application's deployment environment.
server/app/query/servers.tsx (2)
- 90-102: Switching WebSocket URLs from "ws" to "wss" is a crucial security enhancement that ensures encrypted communication between the client and server. This change is correctly implemented across the
logsWebSocketURL,statusWebSocketURL, andstatsWebSocketURLmethods.
- The switch to secure WebSocket URLs ("wss") is correctly implemented and enhances the security of WebSocket communications.
- 274-294: Updating the default URLs for
Coordinator,Helper1,Helper2, andHelper3to use HTTPS instead of HTTP is an important security improvement. It ensures that all communication with these servers is encrypted, protecting against eavesdropping and man-in-the-middle attacks.
- The update to use HTTPS URLs for the default remote server configurations is correctly implemented and significantly improves the security of server communications.
sidecar/app/query/ipa.py (2)
- 164-185: The addition of the
IPAHelperCollectStepsStepclass introduces a new step in the IPA helper query process. The use ofpython3 scripts/collect_steps.py -mwithin thebuild_commandmethod is a good way to automate the collection of steps. However, it's important to ensure that the script is secure and does not introduce any vulnerabilities.
- Verify the security of the
collect_steps.pyscript, especially if it processes external input. Ensure that it does not execute untrusted code or expose sensitive information.
- 402-402: The inclusion of
IPAHelperCollectStepsStepin theIPAHelperQueryclass'sstep_classeslist is a good addition, ensuring that the new step is executed as part of the helper query process. This change appears to be correctly implemented.
- The addition of
IPAHelperCollectStepsStepto theIPAHelperQueryclass is correctly implemented and enhances the functionality of the helper query process.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (3)
- sidecar/app/query/ipa.py (7 hunks)
- sidecar/app/query/step.py (1 hunks)
- sidecar/cli/cli.py (4 hunks)
Files skipped from review as they are similar to previous changes (2)
- sidecar/app/query/step.py
- sidecar/cli/cli.py
Additional comments: 4
sidecar/app/query/ipa.py (4)
- 157-159: The modifications to the
build_commandmethod in theIPAHelperCompileStepclass include additional features for the build process. Ensure these features align with the project's requirements and do not introduce unnecessary dependencies.- 164-185: The addition of the
IPAHelperCollectStepsStepclass introduces a new step in the process. Ensure that this step is necessary and correctly integrated into the workflow. It's important to verify that the scriptscripts/collect_steps.pyexists and functions as expected.- 196-198: The addition of the
pre_runmethod in theIPACoordinatorGenerateTestDataStepclass is a good practice to ensure the necessary directories exist before file operations. This enhances the robustness of the file handling process.- 402-402: The addition of
IPAHelperCollectStepsStepto thestep_classeslist in theIPAHelperQueryclass ensures that the new step is executed as part of the query process. Verify that this integration is intended and correctly implemented.
sidecar/app/query/ipa.py
Outdated
| finish_url = urlunparse( | ||
| helper.sidecar_url._replace( | ||
| scheme="https", path=f"/stop/kill/{self.query_id}" | ||
| ), | ||
| ) | ||
|
|
||
| r = httpx.post( | ||
| finish_url, | ||
| verify=False, |
There was a problem hiding this comment.
Using verify=False in HTTP requests disables SSL certificate verification, posing a significant security risk by making the application vulnerable to man-in-the-middle attacks. This concern was previously raised and remains valid.
- Consider implementing a more secure method for SSL certificate verification. If disabling verification is necessary for development or testing, ensure it is not used in production and is well-documented.
sidecar/app/query/ipa.py
Outdated
| sidecar_urls = [ | ||
| helper.sidecar_url | ||
| for helper in settings.helpers.values() | ||
| if helper.role != Role.COORDINATOR | ||
| ] | ||
| for helper_url in helper_urls: | ||
| for sidecar_url in sidecar_urls: | ||
| url = urlunparse( | ||
| helper_url._replace( | ||
| scheme="ws", path=f"/start/ipa-helper/{self.query_id}/status" | ||
| sidecar_url._replace( | ||
| scheme="https", path=f"/start/ipa-helper/{self.query_id}/status" | ||
| ), | ||
| ) | ||
| while True: | ||
| r = httpx.get(url).json() | ||
| print(url) | ||
| r = httpx.get(url, verify=False).json() |
There was a problem hiding this comment.
The loop within the IPACoordinatorWaitForHelpersStep class's run method uses httpx.get with verify=False, which disables SSL certificate verification. This is a security concern that needs to be addressed.
- Similar to previous comments, consider implementing a secure method for SSL certificate verification. If disabling verification is necessary for development or testing, ensure it is not used in production and is well-documented.
sidecar/app/query/ipa.py
Outdated
| finish_url = urlunparse( | ||
| helper.sidecar_url._replace( | ||
| scheme="https", path=f"/stop/finish/{self.query_id}" | ||
| ), | ||
| ) | ||
|
|
||
| r = httpx.post( | ||
| finish_url, | ||
| json={"identity": str(self.role.value), "signature": signature}, | ||
| verify=False, |
There was a problem hiding this comment.
The use of verify=False in the send_terminate_signals method of the IPACoordinatorQuery class is another instance where SSL certificate verification is disabled. This is a recurring security issue that needs to be addressed across the codebase.
- Recommend implementing a secure method for handling SSL certificate verification. Ensure that disabling verification is not used in production environments and that its use is clearly documented for development or testing purposes.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (3)
- server/app/query/github.tsx (3 hunks)
- sidecar/app/query/base.py (3 hunks)
- sidecar/app/query/ipa.py (8 hunks)
Additional comments: 7
server/app/query/github.tsx (1)
- 26-26: It's good to see the GitHub API key being used for authentication. Ensure that the environment variable name change (
OCTOKIT_GITHUB_API_KEY) is consistently applied across the entire codebase to avoid authentication issues.sidecar/app/query/base.py (2)
- 21-22: The introduction of
QueryExistsErroris a good practice for providing more specific error handling. This makes the code more readable and easier to debug. Ensure that this new exception is adequately documented and that its usage is consistent throughout the codebase.- 72-79: The try-except block used to handle
QueryExistsErrorand avoid race conditions is a thoughtful approach. However, ensure that this pattern does not mask other potential issues that could raise the same exception. It might be beneficial to add more detailed logging within the catch block to provide insight into the frequency and context of these race conditions.sidecar/app/query/ipa.py (4)
- 7-7: The removal of unused imports (
base64,cryptographymodules) is a good practice for keeping the code clean and reducing unnecessary dependencies. Ensure that this change does not affect any other parts of the application that might rely on these imports indirectly.- 161-182: The addition of
IPAHelperCollectStepsStepclass is a good example of extending functionality in a modular way. The methodbuild_commandcorrectly constructs the command for collecting steps. Ensure that the scriptscripts/collect_steps.pyis properly documented and tested, especially since it's being invoked with a specific flag (-m).- 193-195: The
pre_runmethod inIPACoordinatorGenerateTestDataStepis a proactive approach to ensure the output directory exists before running the command. This helps in avoiding runtime errors related to missing directories. Good practice!- 394-394: Adding
IPAHelperCollectStepsStepto the list of step classes inIPAHelperQueryis a clear way to include this new step in the query processing pipeline. Ensure that the integration of this new step is thoroughly tested, especially in scenarios where step execution order is critical.Verification successful
The integration and execution order of
IPAHelperCollectStepsStepwithin the query processing pipeline have been confirmed. It is included in thestep_classeslist insidecar/app/query/ipa.py, positioned afterIPAHelperCompileStepand beforeIPAStartHelperStep. This placement within the list suggests a deliberate order of execution among the steps in the pipeline, aligning with the review comment's emphasis on testing the integration and execution order of the new step.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the integration and execution order of IPAHelperCollectStepsStep in the query processing pipeline. echo "Manual testing is required to ensure the new step integrates correctly and executes in the intended order."Length of output: 222
Script:
#!/bin/bash # Search for references to IPAHelperCollectStepsStep to understand its integration and execution order. rg "IPAHelperCollectStepsStep" -A 5 -B 5 # Additionally, look for files that define the query processing pipeline to inspect the step execution order. rg "query processing pipeline" -A 5 -B 5Length of output: 1039
| // TODO: raise error if api key is expired | ||
| if (process.env.OCTOKIT_GITHUB_API_KEY === undefined) { | ||
| console.warn( | ||
| "WARNING: Octokit requires a personal access token to function properly. Please add OCTOKIT_GITHUB_API_KEY to .env. It does not require any permissions.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Adding a warning for when the GitHub API key is not set is a good practice for clarity and debugging. However, consider also implementing a more robust error handling mechanism for when the API key is expired, as mentioned in the TODO comment. This could involve catching specific exceptions related to API key expiration and providing clear error messages or recovery steps.
Would you like assistance in implementing error handling for expired API keys?
There was a problem hiding this comment.
yes please provide a suggestion. note that the key doesn't require any permissions
There was a problem hiding this comment.
@eriktaubeneck, considering the Octokit client's usage in your application and the requirement that the API key does not need any permissions, you can enhance error handling by wrapping your API calls in a try-catch block. This approach allows you to catch any errors thrown by Octokit, including those related to expired or invalid API keys. Here's a suggestion for how you might implement this:
async function fetchBranches(owner: string, repo: string): Promise<Branch[]> {
try {
const branchesIter = octokit.paginate.iterator(octokit.rest.repos.listBranches, {
owner: owner,
repo: repo,
per_page: 100,
auth: process.env.OCTOKIT_GITHUB_API_KEY,
});
let branchesArray: Branch[] = [];
for await (const { data: branches } of branchesIter) {
for (const branch of branches) {
branchesArray.push({
name: branch.name,
commitHash: branch.commit.sha,
});
}
}
return branchesArray;
} catch (error) {
if (error.status === 401) { // Assuming 401 is the status code for authentication errors including expired tokens
console.error("Authentication error: Please check if your OCTOKIT_GITHUB_API_KEY is expired or invalid.");
} else {
console.error("An unexpected error occurred:", error.message);
}
return []; // Return an empty array or handle the error as appropriate for your application
}
}This example wraps the fetchBranches function (a hypothetical function for fetching branches, as an example) in a try-catch block. It specifically checks for a 401 status code, which is commonly used to indicate authentication errors, including expired or invalid API keys. Adjust the error handling as necessary for your application's needs, including the possibility of re-throwing the error or handling different types of errors differently.
Remember to apply a similar pattern to other Octokit API calls in your application to ensure consistent and robust error handling across your application.
| const mainBranchIndex = branchesArray.findIndex( | ||
| (branch) => branch.name === "main", | ||
| ); | ||
| if (mainBranchIndex != -1) { | ||
| branchesArray.unshift(branchesArray.splice(mainBranchIndex, 1)[0]); |
There was a problem hiding this comment.
The logic to prioritize the main branch by moving it to the front of the branches array is a practical approach for repositories where the main branch is of primary interest. However, adding a placeholder branch with name "N/A" and empty commit hash at the beginning of the array might lead to confusion or errors in downstream processing. Consider documenting this behavior or revising the approach to ensure it aligns with the expectations of consumers of the Branches function.
| if queries.get(self.query_id) is not None: | ||
| raise Exception(f"{self.query_id} already exists") | ||
| raise QueryExistsError(f"{self.query_id} already exists") |
There was a problem hiding this comment.
Raising QueryExistsError when a query already exists is a clear way to handle duplicate queries. This approach helps in avoiding race conditions and makes the error handling more explicit. Consider adding a log statement before raising the exception to aid in debugging and monitoring.
+ self.logger.error(f"Query {self.query_id} already exists");
raise QueryExistsError(f"{self.query_id} already exists")Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if queries.get(self.query_id) is not None: | |
| raise Exception(f"{self.query_id} already exists") | |
| raise QueryExistsError(f"{self.query_id} already exists") | |
| if queries.get(self.query_id) is not None: | |
| self.logger.error(f"Query {self.query_id} already exists"); | |
| raise QueryExistsError(f"{self.query_id} already exists") |
| finish_url = urlunparse( | ||
| helper.sidecar_url._replace( | ||
| scheme="https", path=f"/stop/kill/{self.query_id}" | ||
| ), |
There was a problem hiding this comment.
Replacing urljoin with urlunparse for constructing URLs is a valid approach, especially if it provides more control over URL components. However, using verify=False in HTTP requests (httpx.post and httpx.get) poses a significant security risk by disabling SSL certificate verification. This should be avoided in production code, as it makes the application vulnerable to man-in-the-middle attacks.
Consider using a proper certificate or configuring a custom CA bundle if dealing with self-signed certificates.
… handling to call crash on exception
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (7)
local_dev/config/network.tomlis excluded by:!**/*.tomllocal_dev/config/pub/h1.pemis excluded by:!**/*.pemlocal_dev/config/pub/h1_mk.pubis excluded by:!**/*.publocal_dev/config/pub/h2.pemis excluded by:!**/*.pemlocal_dev/config/pub/h2_mk.pubis excluded by:!**/*.publocal_dev/config/pub/h3.pemis excluded by:!**/*.pemlocal_dev/config/pub/h3_mk.pubis excluded by:!**/*.pub
Files selected for processing (8)
- README.md (3 hunks)
- local_dev/config/h1.key (1 hunks)
- local_dev/config/h1_mk.key (1 hunks)
- local_dev/config/h2.key (1 hunks)
- local_dev/config/h2_mk.key (1 hunks)
- local_dev/config/h3.key (1 hunks)
- sidecar/app/query/base.py (4 hunks)
- sidecar/app/query/command.py (4 hunks)
Files skipped from review due to trivial changes (1)
- local_dev/config/h2_mk.key
Files skipped from review as they are similar to previous changes (2)
- sidecar/app/query/base.py
- sidecar/app/query/command.py
Additional comments: 7
local_dev/config/h1_mk.key (1)
- 1-1: Please ensure that the update to the hash value in
h1_mk.keyis documented, including the reason for the change and its impact on the system. It's crucial for maintaining security and understanding the configuration's history.local_dev/config/h1.key (1)
- 2-4: Updating a private key is a critical security operation. Please ensure that:
- The old key is securely decommissioned.
- The new key is properly protected and its distribution is limited.
- The change is documented, including the reason for the update and any potential impacts on the system.
local_dev/config/h2.key (1)
- 2-4: As with any update to private keys, please ensure the following:
- Secure decommissioning of the old key.
- Proper protection and limited distribution of the new key.
- Documentation of the change, including the reason for the key update and any impacts on the system.
local_dev/config/h3.key (1)
- 2-4: Ensure the update to the private key in
h3.keyfollows best security practices:
- Securely decommission the old key.
- Protect the new key and limit its distribution.
- Document the change, including the reason for the update and any system impacts.
README.md (3)
- 102-138: The instructions for installing Traefik and generating local certificates are clear and helpful. However, ensure that users are aware of the security implications of modifying the
/etc/hostsfile and installing certificates. It might be beneficial to add a note on the potential security considerations when making these changes.- 167-198: The section on generating TLS certificates with Let's Encrypt is detailed and informative. However, the use of
sudowith the virtual environment'scertbotbinary has been previously discussed. It's good to see that this concern has been addressed, but continue to ensure the virtual environment remains secure to prevent privilege escalation.- 209-216: When configuring the system with other parties' domains and public keys, ensure that all information is accurate and securely shared. Misconfiguration or leakage of sensitive information could lead to security vulnerabilities. Consider adding a reminder for users to verify the accuracy and security of the information they input.
| ``` | ||
| NEXT_PUBLIC_SUPABASE_URL="http://localhost:54321" | ||
| NEXT_PUBLIC_SUPABASE_ANON_KEY="<ANON_KEY>" | ||
| NEXT_PUBLIC_SITE_URL=http://localhost:3000 | ||
| NEXT_PUBLIC_SITE_URL="https://draft.test" | ||
| SUPABASE_AUTH_GITHUB_CLIENT_ID="<CLIENT_ID>" | ||
| SUPABASE_AUTH_GITHUB_SECRET="<CLIENT_SECRET>" | ||
| ``` | ||
|
|
||
| **Traefik** | ||
|
|
||
| install traefik | ||
|
|
||
| ``` | ||
| brew install traefik | ||
| ``` | ||
|
|
||
| update /etc/hosts with (requires sudo) | ||
|
|
||
| ``` | ||
| 127.0.0.1 draft.test | ||
| 127.0.0.1 sidecar0.draft.test | ||
| 127.0.0.1 sidecar1.draft.test | ||
| 127.0.0.1 sidecar2.draft.test | ||
| 127.0.0.1 sidecar3.draft.test | ||
| ``` | ||
|
|
||
| make local certs | ||
|
|
||
| install mkcert with | ||
|
|
||
| ``` | ||
| brew install mkcert | ||
| ``` | ||
|
|
||
| make the cert with | ||
|
|
||
| ``` | ||
| mkcert -cert-file "local_dev/config/cert.pem" -key-file "local_dev/config/key.pem" "draft.test" "*.draft.test" | ||
| ``` | ||
|
|
||
| If you get a warning about the cert not being installed (i.e., it's the first time you've used mkcert), also run: | ||
| ``` | ||
| mkcert -install | ||
| ``` | ||
|
|
||
| **Run local dev** | ||
|
|
||
| You're now ready to install, run, and develop on `draft`! | ||
|
|
||
| To start the local development environment: |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [6-7]
There's a grammatical error in the description of draft-sidecar. Consider revising to maintain subject-verb agreement:
- this include a CLI for setup and running.
+ this includes a CLI for setup and running.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (2)
- README.md (4 hunks)
- sidecar/app/query/base.py (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- sidecar/app/query/base.py
Additional comments: 2
README.md (2)
- 102-138: The instructions for installing Traefik, updating the hosts file, and generating local certificates are clear and well-structured. However, ensure that users understand the security implications of modifying the
/etc/hostsfile and installing certificates. It might be beneficial to add a note about the security considerations when making these changes, especially for users unfamiliar with these operations.- 204-212: The instructions for generating TLS certificates with Let's Encrypt are clear and straightforward. However, the use of
sudowith the virtual environment'scertbotbinary was previously discussed. It's good that the clarification was provided in the previous comments, but it might be beneficial to include a brief note in the README itself to preemptively address any potential security concerns users might have regarding this practice.
README.md
Outdated
| @@ -4,7 +4,7 @@ | |||
| draft is a project designed to help test [IPA](https://github.com/private-attribution/ipa) at scale. It contains 2 components: | |||
|
|
|||
| 1. draft-server: a web front end and service that starts queries an displays logs from the MPC helper servers | |||
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. this include a CLI for setup and running. | |||
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. this includes a CLI for setup and running. | |||
There was a problem hiding this comment.
There are a couple of issues in this line:
- Grammatical error: "this include" should be "this includes".
- The sentence does not start with an uppercase letter.
- this includes a CLI for setup and running.
+ This includes a CLI for setup and running.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. this includes a CLI for setup and running. | |
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. This includes a CLI for setup and running. |
README.md
Outdated
| 1. draft-server: a web front end and service that starts queries an displays logs from the MPC helper servers | ||
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. this include a CLI for setup and running. | ||
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. this includes a CLI for setup and running. |
There was a problem hiding this comment.
The phrase "starts queries an displays logs" seems to have a typo. It likely should be "starts queries and displays logs".
- starts queries an displays logs
+ starts queries and displays logsCommittable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| 1. draft-server: a web front end and service that starts queries an displays logs from the MPC helper servers | |
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. this include a CLI for setup and running. | |
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. this includes a CLI for setup and running. | |
| 1. draft-server: a web front end and service that starts queries and displays logs from the MPC helper servers | |
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. this includes a CLI for setup and running. |
| ### IPA specific certs | ||
|
|
||
| We check in self signed certs that are only for local development (and are not secure! They are in a public repo!) | ||
|
|
||
| They will periodically expire. You can regenerate them with a compiled helper binary: | ||
|
|
||
| ``` | ||
| target/release/helper keygen --name localhost --tls-key local_dev/config/h1.key --tls-cert local_dev/config/pub/h1.pem --mk-public-key local_dev/config/pub/h1_mk.pub --mk-private-key local_dev/config/h1_mk.key | ||
| target/release/helper keygen --name localhost --tls-key local_dev/config/h2.key --tls-cert local_dev/config/pub/h2.pem --mk-public-key local_dev/config/pub/h2_mk.pub --mk-private-key local_dev/config/h2_mk.key | ||
| target/release/helper keygen --name localhost --tls-key local_dev/config/h3.key --tls-cert local_dev/config/pub/h3.pem --mk-public-key local_dev/config/pub/h3_mk.pub --mk-private-key local_dev/config/h3_mk.key | ||
| ``` | ||
|
|
||
| The public content will also need to be pasted into `local_dev/config/network.toml` for each helper. |
There was a problem hiding this comment.
The section on IPA-specific certificates mentions that self-signed certificates are checked in for local development. This is a security concern, even if it's only for local development. It's crucial to ensure that users are aware these certificates should never be used in production environments. Consider adding a prominent warning about the security implications of using self-signed certificates and the importance of using properly issued certificates for any environment outside of local development.
README.md
Outdated
| 5. Generate IPA specific keys: | ||
| 1. TODO |
There was a problem hiding this comment.
The TODO under "Generate IPA specific keys" is left unaddressed in the README. It's important to either complete this section with the necessary instructions or remove the placeholder if it's not applicable. Leaving TODOs in documentation can lead to confusion for users trying to follow the setup instructions.
Would you like assistance in completing this section, or should it be removed?
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- README.md (4 hunks)
Additional comments: 3
README.md (3)
- 102-138: The instructions for installing Traefik and generating local certificates are clear and concise. However, it's important to ensure that users understand the security implications of using self-signed certificates and the necessity of replacing them with properly issued certificates for production environments. Consider adding a warning or note about this.
- 166-178: The section on IPA-specific certificates correctly warns about the security implications of using self-signed certificates for local development. However, it would be beneficial to emphasize more strongly that these certificates should never be used in production environments. Consider adding a more prominent warning.
- 230-236: When generating IPA-specific keys, it's crucial to ensure that the process is secure and that private keys are handled with care. The instructions provided are clear, but it may be beneficial to add a note about the importance of securing private keys and ensuring they are not exposed or checked into version control.
| 1. draft-server: a web front end and service that starts queries an displays logs from the MPC helper servers | ||
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. this include a CLI for setup and running. | ||
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. This includes a CLI for setup and running. |
There was a problem hiding this comment.
There seems to be a grammatical error in the description of draft-sidecar. The verb form does not agree with the singular subject "sidecar back end API".
- This includes a CLI for setup and running.
+ This includes a CLI for setup and running, and it runs next to the IPA binary on helper servers.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| 1. draft-server: a web front end and service that starts queries an displays logs from the MPC helper servers | |
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. this include a CLI for setup and running. | |
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. This includes a CLI for setup and running. | |
| 1. draft-server: a web front end and service that starts queries an displays logs from the MPC helper servers | |
| 2. draft-sidecar: a sidecar back end API that runs next to the IPA binary on helper servers. This includes a CLI for setup and running, and it runs next to the IPA binary on helper servers. |
| You will need a domain name and TLS certificates for the sidecar to properly run over HTTPS. The following instructions assume your domain is `example.com`, please replace with the domain you'd like to use. You will need to create two sub-domains, `sidecar.example.com` and `helper.example.com`. (Note, you could also use a sub-domain as your base domain, e.g., `test.example.com` with two sub-domains of that: `sidecar.test.example.com` and `helper.test.example.com`.) | ||
|
|
||
| 1. Set up DNS records for `sidecar.example.com` and `helper.example.com` pointing to a server you control. | ||
| 2. Make sure you've installed the requirements above, and are using the virtual environment. | ||
| 3. Install `certbot`: `pip install certbot` | ||
| 4. `sudo .venv/bin/certbot certonly --standalone -m [email protected] -d "sidecar.example.com,helper.example.com"` | ||
| 1. Note that you must point directly to `.venv/bin/certbot` as `sudo` does not operate in the virtualenv. | ||
| 5. Accept the [Let's Encrypt terms](https://letsencrypt.org/documents/LE-SA-v1.3-September-21-2022.pdf). | ||
|
|
||
|
|
||
| ### Make Configuration |
There was a problem hiding this comment.
The instructions for generating TLS certificates with Let's Encrypt are well-detailed. However, there's a potential security concern with the use of sudo in the command for running certbot. It's recommended to use a system-wide installation of certbot rather than invoking it through the virtual environment with sudo. This approach reduces the risk associated with elevated privileges and ensures that certbot operates in a more controlled environment.
- sudo .venv/bin/certbot certonly --standalone -m [email protected] -d "sidecar.example.com,helper.example.com"
+ sudo certbot certonly --standalone -m [email protected] -d "sidecar.example.com,helper.example.com"Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| You will need a domain name and TLS certificates for the sidecar to properly run over HTTPS. The following instructions assume your domain is `example.com`, please replace with the domain you'd like to use. You will need to create two sub-domains, `sidecar.example.com` and `helper.example.com`. (Note, you could also use a sub-domain as your base domain, e.g., `test.example.com` with two sub-domains of that: `sidecar.test.example.com` and `helper.test.example.com`.) | |
| 1. Set up DNS records for `sidecar.example.com` and `helper.example.com` pointing to a server you control. | |
| 2. Make sure you've installed the requirements above, and are using the virtual environment. | |
| 3. Install `certbot`: `pip install certbot` | |
| 4. `sudo .venv/bin/certbot certonly --standalone -m [email protected] -d "sidecar.example.com,helper.example.com"` | |
| 1. Note that you must point directly to `.venv/bin/certbot` as `sudo` does not operate in the virtualenv. | |
| 5. Accept the [Let's Encrypt terms](https://letsencrypt.org/documents/LE-SA-v1.3-September-21-2022.pdf). | |
| ### Make Configuration | |
| You will need a domain name and TLS certificates for the sidecar to properly run over HTTPS. The following instructions assume your domain is `example.com`, please replace with the domain you'd like to use. You will need to create two sub-domains, `sidecar.example.com` and `helper.example.com`. (Note, you could also use a sub-domain as your base domain, e.g., `test.example.com` with two sub-domains of that: `sidecar.test.example.com` and `helper.test.example.com`.) | |
| 1. Set up DNS records for `sidecar.example.com` and `helper.example.com` pointing to a server you control. | |
| 2. Make sure you've installed the requirements above, and are using the virtual environment. | |
| 3. Install `certbot`: `pip install certbot` | |
| 4. `sudo certbot certonly --standalone -m [email protected] -d "sidecar.example.com,helper.example.com"` | |
| 1. Note that you must point directly to `.venv/bin/certbot` as `sudo` does not operate in the virtualenv. | |
| 5. Accept the [Let's Encrypt terms](https://letsencrypt.org/documents/LE-SA-v1.3-September-21-2022.pdf). | |
| ### Make Configuration |
This adds
traefikin front of the python sidecar application, which handles TLS. This is required for an HTTPS website to POST to the sidecar.It also includes a few small bug fixes to get the helpers deployed on AWS.
Summary by CodeRabbit
QueryExistsErrorto improve error handling for existing queries.cwdfield to theCommandclass for specifying the current working directory in subprocess execution.FileOutputCommandclass to improve efficiency.